Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix CI build #6165

Merged
merged 1 commit into from
Sep 12, 2019
Merged

Fix CI build #6165

merged 1 commit into from
Sep 12, 2019

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented Sep 11, 2019 •
edited by vitaliy-guliy
Loading

Signed-off-by: Oleksii Orel oorel@redhat.com

What it does

Fix CI build.

How to test

Review checklist

Reminder for reviewers

@olexii4 olexii4 requested a review from AlexTugarev September 11, 2019 15:38
@olexii4
Copy link
Contributor Author

olexii4 commented Sep 11, 2019

I can't reproduce it locally. So, I am going to test it with CI.

@olexii4 olexii4 changed the title Fix CI build [WIP] Fix CI build Sep 11, 2019
@marcdumais-work
Copy link
Contributor

Hi @olexii4 . FYI the repo was just moved to the eclipse-theia organization and CI is not completely configured yet. so issues are expected.

@olexii4 olexii4 changed the title [WIP] Fix CI build Fix CI build Sep 11, 2019
@olexii4 olexii4 mentioned this pull request Sep 11, 2019
Merged
@kittaakos
Copy link
Contributor

I can't reproduce it locally

I could not reproduce it locally either.

FYI the repo was just moved to the eclipse-theia organization and CI is not completely configured yet

Perfect timing 😄. Shall we revert the PR?

@kittaakos
Copy link
Contributor

I can't reproduce it locally.

Nice that you found the fix, but can you explain why it worked locally and not from the CIs?

@olexii4
Copy link
Contributor Author

olexii4 commented Sep 11, 2019

@kittaakos I cannot tell exactly(ideas only). I just fixed an error from the CI log.

@olexii4 olexii4 requested a review from kittaakos September 11, 2019 17:41
@kittaakos
Copy link
Contributor

I can't reproduce it locally

I could not reproduce it locally either.

It was a test failure. We do not run the tests :/

I cannot tell exactly(ideas only).

I did not know it either.

When we load the connection-status-service.spec module with mocha the following modules will be required and loaded: connection-status-service > frontend-application > shell/shell-layout-restorer > theming. The required object exists. It cannot be an incorrect CSS file path or config issue in mocha.

require('../../src/browser/style/materialcolors.css')
Object {}
[[StableObjectId]]:8
__proto__:Object {constructor: , __defineGetter__: , __defineSetter__: , …}
__defineGetter__:function __defineGetter__() { … }
__defineSetter__:function __defineSetter__() { … }
__lookupGetter__:function __lookupGetter__() { … }
__lookupSetter__:function __lookupSetter__() { … }
[[StableObjectId]]:9
constructor:function Object() { … }
hasOwnProperty:function hasOwnProperty() { … }
isPrototypeOf:function isPrototypeOf() { … }
propertyIsEnumerable:function propertyIsEnumerable() { … }
toLocaleString:function toLocaleString() { … }
toString:function toString() { … }
valueOf:function valueOf() { … }
__proto__:null

(from debug console ☝️)

However, the style-loader does not work in mocha tests.

vince-fugnitto
vince-fugnitto reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce it locally. So, I am going to test it with CI.

@olexii4 I can confirm that it fails on master when executing the tests:

$ npx run test @theia/core

...

/Users/vincentfugnitto/workspace/theia/packages/core/lib/browser/theming.js:43
require('../../src/browser/style/materialcolors.css').use();
                                                      ^

TypeError: require(...).use is not a function
    at Object.<anonymous> (/Users/vincentfugnitto/workspace/theia/packages/core/lib/browser/theming.js:43:55)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at Module.replacementCompile (/Users/vincentfugnitto/workspace/theia/node_modules/nyc/node_modules/append-transform/index.js:58:13)
    at module.exports (/Users/vincentfugnitto/workspace/theia/node_modules/nyc/node_modules/default-require-extensions/js.js:8:9)
    at Object.<anonymous> (/Users/vincentfugnitto/workspace/theia/node_modules/nyc/node_modules/append-transform/index.js:62:4)
    at Module.load (internal/modules/cjs/loader.js:600:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:539:12)
    at Function.Module._load (internal/modules/cjs/loader.js:531:3)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at Module.require (/Users/vincentfugnitto/workspace/theia/node_modules/monaco-languageclient/lib/register-vscode.js:12:28)
    at require (internal/modules/cjs/helpers.js:22:18)

...

On a separate note, can you update the PR message and description to accurately describe the fix?
Looking back at the PR in history, "Fix CI build" won't say much as to what the problem was (in an effort to not repeat the problem again).

@akosyakov akosyakov added the ci issues related to CI / tests label Sep 12, 2019
@akosyakov akosyakov force-pushed the CHE-14196_1 branch 2 times, most recently from 2ba7b28 to c934033 Compare September 12, 2019 04:09
akosyakov
akosyakov approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's merge as soon as travis is green

@akosyakov
fix CI build
a7259d9 
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov akosyakov merged commit c70d6a3 into master Sep 12, 2019
@akosyakov akosyakov deleted the CHE-14196_1 branch September 12, 2019 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@akosyakov akosyakov akosyakov approved these changes

@AlexTugarev AlexTugarev Awaiting requested review from AlexTugarev

@kittaakos kittaakos Awaiting requested review from kittaakos

Assignees
No one assigned
Labels
ci issues related to CI / tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@olexii4 @marcdumais-work @kittaakos @akosyakov @vince-fugnitto